feat!: remove square size from block data #1051

rach-id · 2023-07-28T10:47:36Z

Description

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use
unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

evan-forbes

wait, why are we removing the data hash from block data? Isn't this how we are passing the data root from the app back to core?

rach-id · 2023-07-30T09:07:21Z

I guess that changed, and we're using this:

celestia-core/state/execution.go

Lines 139 to 151 in d02553f

    
           // Celestia passes the data root back as the second to last transaction 
        
           // and the big endian encoding of the square size as the last transaction. 
        
           if len(rpp.Txs) < 2 { 
        
           	panic("state machine returned an invalid prepare proposal response: expected at least 2 transactions") 
        
           } 
        
           if len(rpp.Txs[len(rpp.Txs)-2]) != tmhash.Size { 
        
           	panic(fmt.Sprintf("state machine returned an invalid prepare proposal response: expected second to last transaction to be a hash, got %d bytes", len(rpp.Txs[len(rpp.Txs)-2]))) 
        
           } 
        
           if len(rpp.Txs[len(rpp.Txs)-1]) != 8 { 
        
           	panic("state machine returned an invalid prepare proposal response: expected last transaction to be a uint64 (square size)") 
        
           }

Note: the PR is to main and not to current releases branch

evan-forbes · 2023-07-30T10:46:22Z

types/block.go

-	data.SquareSize = dp.SquareSize
-	data.hash = dp.Hash


this is what I was referring to @sweexordious

if we don't do this, then when we call data.Hash()

celestia-core/types/block.go

Lines 1025 to 1037 in d02553f

// Hash returns the hash of the data

func (data *Data) Hash() cmtbytes.HexBytes {

if data == nil {

return (Txs{}).Hash()

}

if data.hash == nil {

data.hash = data.Txs.Hash() // NOTE: leaves of merkle tree are TxIDs

}

// this is the expected behavior where `data.hash` was set by celestia-app

// in PrepareProposal

return data.hash

}

we will not use the celestia-app version of the hash

this change is ignoring the hash we're passing back and instead we're using the standard tendermint one which is why the tests pass fine (that's actually the only reason we allow this to occur at all)

it certainly doesn't have to be in this PR, but since this mechanism is so hidden it would probably be a good idea to include a test to ensure that we don't miss this in the future. Currently, those tests are only in the application, but we should be able to modify one of the test applications here to modify the data root

Good catch 🙏

what do you think about this? f84fa8a

We could either do this, or move those two lines that set the data hash and square size to the DataFromProto method. However, we will also need to change:

celestia-core/state/execution.go

Lines 154 to 156 in 842a0b5

block.Data, _ = types.DataFromProto(&cmtproto.Data{

Txs: rpp.Txs[:len(rpp.Txs)-2],

})

to take the whole rpp.Txs slice with the square size and the data hash appended at the end.

What do you think?

we call data.Hash() in other places, therefore we are unable to only set the value in the header

if we change one of the test applications to also modify the header, then we will be able to test this issue

cmwaters · 2023-07-31T07:31:32Z

Theoretically it is possible to remove the hash field from the data struct so long as the data root can be directly added to the header.

rach-id · 2023-07-31T12:22:22Z

Theoretically it is possible to remove the hash field from the data struct so long as the data root can be directly added to the header.

Agreed. But I checked comet and apparently they still have that in the types.Data struct. Also, in this PR I am leaving the square size there because it is being used here:

celestia-core/state/execution.go

Line 180 in d02553f

txs := append(block.Data.Txs.ToSliceOfBytes(), block.DataHash, squareSizeBytes)

state/execution.go

proto/tendermint/types/types.proto

evan-forbes

thanks for fixing 👍

cmwaters

approving my own changes like...

evan-forbes

one last hardcoded test I think

TestBlockchainMessageVectors

cmwaters · 2023-08-24T15:00:55Z

Yup I'm on it

feat!: remove square size and data hash from block data

842a0b5

rach-id added C:types Component: types breaking labels Jul 28, 2023

rach-id requested review from evan-forbes and cmwaters as code owners July 28, 2023 10:47

rach-id self-assigned this Jul 28, 2023

evan-forbes requested changes Jul 30, 2023

View reviewed changes

evan-forbes reviewed Jul 30, 2023

View reviewed changes

evan-forbes mentioned this pull request Jul 30, 2023

Create a test to ensure the data root can be changed by the application #1053

Closed

evan-forbes mentioned this pull request Jul 31, 2023

Stop returning the square size in PrepareProposal celestiaorg/celestia-app#2191

Open

fix: set the data hash and square size in create proposal block

f84fa8a

evan-forbes mentioned this pull request Aug 2, 2023

Remove the data hash and square size from Data #1045

Closed

cmwaters reviewed Aug 8, 2023

View reviewed changes

state/execution.go Outdated Show resolved Hide resolved

cmwaters reviewed Aug 8, 2023

View reviewed changes

proto/tendermint/types/types.proto Outdated Show resolved Hide resolved

cmwaters added 2 commits August 22, 2023 18:51

remove square size from the abci parsing

6ae7e20

fix process proposal

8859c76

cmwaters force-pushed the remove_hash_and_square_size_from_data branch from e20e0f1 to 8859c76 Compare August 23, 2023 12:59

evan-forbes previously approved these changes Aug 23, 2023

View reviewed changes

evan-forbes changed the title ~~feat!: remove square size and data hash from block data~~ feat!: remove square size from block data Aug 23, 2023

cmwaters added 3 commits August 23, 2023 16:35

Merge branch 'main' into remove_hash_and_square_size_from_data

c67a5f3

Merge branch 'main' into remove_hash_and_square_size_from_data

95c1fe1

fix tests

2a5cf84

cmwaters dismissed evan-forbes’s stale review via 2a5cf84 August 24, 2023 14:09

cmwaters requested a review from evan-forbes August 24, 2023 14:10

cmwaters previously approved these changes Aug 24, 2023

View reviewed changes

evan-forbes previously approved these changes Aug 24, 2023

View reviewed changes

fix TestBlockchainMessageVectors

6c675e6

cmwaters dismissed stale reviews from evan-forbes and themself via 6c675e6 August 24, 2023 15:04

evan-forbes approved these changes Aug 24, 2023

View reviewed changes

cmwaters approved these changes Aug 24, 2023

View reviewed changes

cmwaters merged commit 8f3e050 into celestiaorg:main Aug 25, 2023
15 checks passed

rootulp mentioned this pull request Mar 20, 2024

fix: setting data root in CreateProposalBlock #1271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: remove square size from block data #1051

feat!: remove square size from block data #1051

rach-id commented Jul 28, 2023

evan-forbes left a comment

rach-id commented Jul 30, 2023

evan-forbes Jul 30, 2023

evan-forbes Jul 30, 2023

evan-forbes Jul 30, 2023

evan-forbes Jul 30, 2023

rach-id Jul 31, 2023

evan-forbes Aug 2, 2023

cmwaters commented Jul 31, 2023

rach-id commented Jul 31, 2023

evan-forbes left a comment

cmwaters left a comment

evan-forbes left a comment •

edited

Loading

cmwaters commented Aug 24, 2023

	// Hash returns the hash of the data
	func (data *Data) Hash() cmtbytes.HexBytes {
	if data == nil {
	return (Txs{}).Hash()
	}
	if data.hash == nil {
	data.hash = data.Txs.Hash() // NOTE: leaves of merkle tree are TxIDs
	}

	// this is the expected behavior where `data.hash` was set by celestia-app
	// in PrepareProposal
	return data.hash
	}

	block.Data, _ = types.DataFromProto(&cmtproto.Data{
	Txs: rpp.Txs[:len(rpp.Txs)-2],
	})

feat!: remove square size from block data #1051

feat!: remove square size from block data #1051

Conversation

rach-id commented Jul 28, 2023

Description

PR checklist

evan-forbes left a comment

Choose a reason for hiding this comment

rach-id commented Jul 30, 2023

evan-forbes Jul 30, 2023

Choose a reason for hiding this comment

evan-forbes Jul 30, 2023

Choose a reason for hiding this comment

evan-forbes Jul 30, 2023

Choose a reason for hiding this comment

evan-forbes Jul 30, 2023

Choose a reason for hiding this comment

rach-id Jul 31, 2023

Choose a reason for hiding this comment

evan-forbes Aug 2, 2023

Choose a reason for hiding this comment

cmwaters commented Jul 31, 2023

rach-id commented Jul 31, 2023

evan-forbes left a comment

Choose a reason for hiding this comment

cmwaters left a comment

Choose a reason for hiding this comment

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

cmwaters commented Aug 24, 2023

evan-forbes left a comment •

edited

Loading